Implement /plaintext/2.0.0 #1236

ackintosh · 2019-09-02T03:44:54Z

This PR closes #1208 🚀

TODO:

Keep PlainTextConfig as PlainText1Config
- put the new config as PlainText2Config
Validation
- the given peer id is consistent with the given public key

tomaka

Looks good!
I think, however, that we should keep plaintext 1, as this is a very low-maintenance low-effort burden.
Could you keep the current PlainTextConfig and name it PlainText1Config, and name your new object the PlainText2Config?

tomaka · 2019-09-03T15:39:02Z

protocols/plaintext/src/lib.rs

+}
+
+pub struct PlainTextMiddleware<S> {
+    pub inner: Framed<S, BytesMut>,


That shouldn't be public.

tomaka · 2019-09-03T15:39:47Z

protocols/plaintext/src/lib.rs

+
+    fn upgrade_outbound(self, socket: Negotiated<C>, _: Self::Info) -> Self::Future {
+        Box::new(self.handshake(socket))
+//        future::ok(i)


Commented-out code that should be removed.

tomaka · 2019-09-03T15:39:49Z

protocols/plaintext/src/lib.rs

+
+    fn upgrade_inbound(self, socket: Negotiated<C>, _: Self::Info) -> Self::Future {
+        Box::new(self.handshake(socket))
+//        future::ok(i)


Commented-out code that should be removed.

tomaka · 2019-09-03T15:40:47Z

protocols/plaintext/src/lib.rs

+#[derive(Clone)]
+pub struct PlainTextConfig {
+//    peerId: PeerId,
+    pub key: identity::Keypair,


I'd prefer if this was a PublicKey instead of a Keypair.

tomaka · 2019-09-03T15:40:55Z

protocols/plaintext/src/lib.rs

+
+#[derive(Clone)]
+pub struct PlainTextConfig {
+//    peerId: PeerId,


Commented out code to remove.

ackintosh · 2019-09-04T12:00:07Z

Thank you for your review!

I think, however, that we should keep plaintext 1, as this is a very low-maintenance low-effort burden.

That makes sense. 💡

I'll update entire the codes to compatible to the plaintext/2.0.0 spec (and to include your advice) as this PR is still draft. 😃

ackintosh · 2019-09-04T12:16:28Z

I've added a TODO section to the first comment on this PR. 📝

ackintosh · 2019-09-06T13:24:27Z

I've tested that examples/ping works fine with the sample code below:

sample code based on libp2p::::build_tcp_ws_secio_mplex_yamux()

pub fn build_tcp_ws_plaintext_mplex_yamux(keypair: identity::Keypair)
    -> impl Transport<Output = (PeerId, impl core::muxing::StreamMuxer<OutboundSubstream = impl Send, Substream = impl Send, Error = impl Into<io::Error>> + Send + Sync), Error = impl error::Error + Send, Listener = impl Send, Dial = impl Send, ListenerUpgrade = impl Send> + Clone
{
    // An another keys for testing the validation
    let _another_keys = identity::Keypair::generate_ed25519();

    CommonTransport::new()
        .with_upgrade(PlainText2Config { pubkey: keypair.public(), peer_id: keypair.public().into_peer_id() })
        .and_then(move |output, endpoint| {
            let peer_id = output.remote_key.into_peer_id();
            let peer_id2 = peer_id.clone();
            let upgrade = core::upgrade::SelectUpgrade::new(yamux::Config::default(), mplex::MplexConfig::new())
                // TODO: use a single `.map` instead of two maps
                .map_inbound(move |muxer| (peer_id, muxer))
                .map_outbound(move |muxer| (peer_id2, muxer));
            core::upgrade::apply(output.stream, upgrade, endpoint)
                .map(|(id, muxer)| (id, core::muxing::StreamMuxerBox::new(muxer)))
        })
        .with_timeout(Duration::from_secs(20))
}

example/ping.rs

// ...
// ...
-    let transport = libp2p::build_development_transport(id_keys);
+    let transport = libp2p::build_tcp_ws_plaintext_mplex_yamux(id_keys);
// ...
// ...

Yes, there is no test codes about plaintext/2.0.0 yet. I'm going to add test codes in another PR. 😄💦

ackintosh · 2019-09-06T13:35:05Z

https://circleci.com/gh/libp2p/rust-libp2p/15485?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

error[E0599]: no variant or associated item named Rsa found for type libp2p_core::identity::PublicKey in the current scope
--> protocols/plaintext/src/handshake.rs:86:24
|
86 | PublicKey::Rsa(_) => KeyType::RSA,
| ^^^ variant or associated item not found in libp2p_core::identity::PublicKey

Hmm... I couldn't find how fix it. In my local machine, it is done successfully. 🙄
@tomaka I'd appreciate it if you could help me. 🙏🙏

ackintosh · 2019-09-07T08:48:36Z

error[E0599]: no variant or associated item named Rsa found for type libp2p_core::identity::PublicKey in the current scope

It seems that the error is related to this 👀 :

rust-libp2p/core/src/identity.rs

Lines 24 to 25 in 057b379

    
           #[cfg(not(any(target_os = "emscripten", target_os = "unknown")))] 
        
           pub mod rsa;

but still I can't find how fix the error as I'm new to Rust and WASM...

ackintosh · 2019-09-07T09:40:53Z

Ok, the CI has passed!

@tomaka This PR is ready for review. 🙏

protocols/plaintext/src/handshake.rs

tomaka · 2019-09-09T07:47:06Z

protocols/plaintext/src/lib.rs

@@ -55,3 +70,130 @@ impl<C> OutboundUpgrade<C> for PlainTextConfig {
    }
 }

+#[derive(Clone)]
+pub struct PlainText2Config {
+    pub peer_id: PeerId,


I suppose that's the remote's PeerId? If so that should be removed.

From the other conversation, this seems to be our local PeerId. I think it should be automatically generated from the pubkey then, and not provided by the configuration.

Deleted the PeerId via abe8499

tomaka · 2019-09-09T07:47:32Z

protocols/plaintext/src/lib.rs

+#[derive(Clone)]
+pub struct PlainText2Config {
+    pub peer_id: PeerId,
+    pub pubkey: identity::PublicKey,


I guess that's our local public key? This fields needs some documentation explaining what it is.

Yes, that is our local public key. Is it better to rename from pubkey to local_pubkey ?

Yes please, rename to local_public_key.

romanb · 2019-09-09T08:18:12Z

As a side note, if #1240 is accepted, it would be nice if the output of the Plaintext2Config is (PeerId, PlaintextOutput), so that it can be directly plugged into transport::upgrade::Builder::authenticate from #1240. This was one of the main motivations for the Plaintext v2 protocol - that it can be used as an (insecure, development-only) "authenticator" / "security protocol". It can also be done as a follow-up, of course.

Co-Authored-By: Roman Borschel <romanb@users.noreply.github.com>

ackintosh · 2019-09-30T14:04:06Z

https://circleci.com/gh/libp2p/rust-libp2p/15741?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

ci/circleci: test-win32

error[E0721]: await is a keyword in the 2018 edition
--> /usr/local/cargo/registry/src/gh.neting.cc-1ecc6299db9ec823/wasm-bindgen-backend-0.2.51/src/codegen.rs:454:79
|
454 | wasm_bindgen::__rt::IntoJsResult::into_js_result(#ret.await)
| ^^^^^ help: you can use a raw identifier to stay compatible: r#await

It seems that the Docker image should be updated. cc: @tomaka
https://hub.docker.com/r/tomaka/rust-mingw-docker

$ docker run --rm tomaka/rust-mingw-docker cargo -V
cargo 1.34.0 (6789d8a0a 2019-04-01)

ackintosh · 2019-09-30T14:11:22Z

I've finished to update this PR but the CI failed, maybe the failure is not related to changes on this PR.

ackintosh · 2019-09-30T23:05:27Z

The failure will be fixed in #1261

ackintosh · 2019-10-02T01:18:00Z

Thank you @tomaka , I've confirmed the CI has passed.

This PR is ready for your review. @tomaka @romanb 🙏

romanb

Generally looks good to me now, I only made one more suggestion. Thanks for your effort @ackintosh.

romanb · 2019-10-06T15:06:38Z

protocols/plaintext/src/error.rs

+
+    /// Failed to parse one of the handshake protobuf messages.
+    HandshakeParsingFailure,
+


Another option is to collapse HandshakeParsingFailure and ProtobufError into the variant

InvalidPayload(Option<ProtobufError>)

Just a thought, because it seems weird to me to have two different error variants for basically the same kind of error (that all or part of the received message payload is invalid / cannot be parsed).

…to plaintext

ackintosh · 2019-10-11T00:06:09Z

@romanb Thanks for your review! I've updated.

romanb · 2019-10-11T07:49:16Z

@tomaka Is this OK to merge from your point of view?

tomaka · 2019-10-11T09:24:33Z

Yes!

tomaka

GitHub prevents me from merging if I don't explicitly approve 😅

* Configurable multistream-select protocol. Add V1Lazy variant. (#1245) Make the multistream-select protocol (version) configurable on transport upgrades as well as for individual substreams. Add a "lazy" variant of multistream-select 1.0 that delays sending of negotiation protocol frames as much as possible but is only safe to use under additional assumptions that go beyond what is required by the multistream-select v1 specification. * Improve the code readability of the chat example (#1253) * Add bridged chats (#1252) * Try fix CI (#1261) * Print Rust version on CI * Don't print where not appropriate * Change caching strategy * Remove win32 build * Remove win32 from list * Update libsecp256k1 dep to 0.3.0 (#1258) * Update libsecp256k1 dep to 0.3.0 * Sign now cannot fail * Upgrade url and percent-encoding deps to 2.1.0 (#1267) * Upgrade percent-encoding dep to 2.1.0 * Upgrade url dep to 2.1.0 * Revert CIPHERS set to null (#1273) * Update dependency versions (#1265) * Update versions of many dependencies * Bump version of rand * Updates for changed APIs in rand, ring, and webpki * Replace references to `snow::Session` `Session` no longer exists in `snow` but the replacement is two structs `HandshakeState` and `TransportState` Something will have to be done to harmonize `NoiseOutput.session` * Add precise type for UnparsedPublicKey * Update data structures/functions to match new snow's API * Delete diff.diff Remove accidentally committed diff file * Remove commented lines in identity/rsa.rs * Bump libsecp256k1 to 0.3.1 * Implement /plaintext/2.0.0 (#1236) * WIP * plaintext/2.0.0 * Refactor protobuf related issues to compatible with the spec * Rename: new PlainTextConfig -> PlainText2Config * Keep plaintext/1.0.0 as PlainText1Config * Config contains pubkey * Rename: proposition -> exchange * Add PeerId to Exchange * Check the validity of the remote's `Exchange` * Tweak * Delete unused import * Add debug log * Delete unused field: public_key_encoded * Delete unused field: local * Delete unused field: exchange_bytes * The inner instance should not be public * identity::Publickey::Rsa is not available on wasm * Delete PeerId from Config as it should be generated from the pubkey * Catch up for #1240 * Tweak * Update protocols/plaintext/src/error.rs Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com> * Update protocols/plaintext/src/handshake.rs Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com> * Update protocols/plaintext/src/error.rs Co-Authored-By: Pierre Krieger <pierre.krieger1708@gmail.com> * Update protocols/plaintext/src/error.rs Co-Authored-By: Roman Borschel <romanb@users.noreply.github.com> * Update protocols/plaintext/src/error.rs Co-Authored-By: Roman Borschel <romanb@users.noreply.github.com> * Rename: pubkey -> local_public_key * Delete unused error * Rename: PeerIdValidationFailed -> InvalidPeerId * Fix: HandShake -> Handshake * Use bytes insteadof Publickey to avoid code duplication * Replace with ProtobufError * Merge HandshakeContext<()> into HandshakeContext<Local> * Improve the peer ID validation to simplify the handshake * Propagate Remote to allow extracting the PeerId from the Remote * Collapse the same kind of errors into the variant * [noise]: `sodiumoxide 0.2.5` (#1276) Fixes rustsec/advisory-db#192 * examples/ipfs-kad.rs: Remove outdated reference to `without_init` (#1280) * CircleCI Test Fix (#1282) * Disabling "Docker Layer Caching" because it breaks one of the circleci checks * Bump to trigger CircleCI build * unbump * zeroize: Upgrade to v1.0 (#1284) v1.0 final release is out. Release notes: iqlusioninc/crates#279 * *: Consolidate protobuf scripts and update to rust-protobuf 2.8.1 (#1275) * *: Consolidate protobuf generation scripts * *: Update to rust-protobuf 2.8.1 * *: Mark protobuf generated modules with '_proto' * examples: Add distributed key value store (#1281) * examples: Add distributed key value store This commit adds a basic distributed key value store supporting GET and PUT commands using Kademlia and mDNS. * examples/distributed-key-value-store: Fix typo * Simple Warning Cleanup (#1278) * Cleaning up warnings - removing unused `use` * Cleaning up warnings - unused tuple value * Cleaning up warnings - removing dead code * Cleaning up warnings - fixing deprecated name * Cleaning up warnings - removing dead code * Revert "Cleaning up warnings - removing dead code" This reverts commit f18a765. * Enable the std feature of ring (#1289)

ackintosh added 2 commits September 2, 2019 12:42

WIP

e686c2f

plaintext/2.0.0

6ba9ca7

tomaka suggested changes Sep 3, 2019

View reviewed changes

Refactor protobuf related issues to compatible with the spec

cfe8cb5

ackintosh force-pushed the plaintext branch from 5ee673b to cfe8cb5 Compare September 4, 2019 13:24

ackintosh added 14 commits September 5, 2019 20:45

Rename: new PlainTextConfig -> PlainText2Config

f8ff1a6

Keep plaintext/1.0.0 as PlainText1Config

8f0a332

Config contains pubkey

bc0e506

Rename: proposition -> exchange

0df77db

Add PeerId to Exchange

63380bf

Check the validity of the remote's Exchange

f67d038

Tweak

8ca690b

Merge remote-tracking branch 'upstream/master' into plaintext

279b053

Delete unused import

8155338

Add debug log

037da03

Delete unused field: public_key_encoded

9b4c606

Delete unused field: local

b8ae8c4

Delete unused field: exchange_bytes

be57a76

The inner instance should not be public

f0e5a18

ackintosh marked this pull request as ready for review September 6, 2019 13:36

identity::Publickey::Rsa is not available on wasm

7b35c0d

tomaka suggested changes Sep 9, 2019

View reviewed changes

protocols/plaintext/src/handshake.rs Outdated Show resolved Hide resolved

tomaka suggested changes Sep 9, 2019

View reviewed changes

ackintosh and others added 11 commits September 28, 2019 21:28

Update protocols/plaintext/src/error.rs

e4b56fd

Co-Authored-By: Roman Borschel <romanb@users.noreply.github.com>

Rename: pubkey -> local_public_key

15f5320

Delete unused error

76a1842

Rename: PeerIdValidationFailed -> InvalidPeerId

5282e84

Fix: HandShake -> Handshake

af56ce4

Use bytes insteadof Publickey to avoid code duplication

354a2e3

Replace with ProtobufError

1850fca

Merge HandshakeContext<()> into HandshakeContext<Local>

dbdf4e8

Improve the peer ID validation to simplify the handshake

131740a

Propagate Remote to allow extracting the PeerId from the Remote

c9157f7

Merge branch 'master' into plaintext

58a9c45

Merge branch 'master' into plaintext

0ba3d48

romanb approved these changes Oct 6, 2019

View reviewed changes

ackintosh added 3 commits October 11, 2019 08:22

Merge branch 'master' into plaintext

452e4f3

Collapse the same kind of errors into the variant

1ad1b6b

Merge branch 'plaintext' of ssh://github.com/ackintosh/rust-libp2p in…

97fb0ca

…to plaintext

Merge branch 'master' into plaintext

228744a

tomaka approved these changes Oct 11, 2019

View reviewed changes

tomaka merged commit a61ad92 into libp2p:master Oct 11, 2019

ackintosh deleted the plaintext branch October 17, 2019 10:49

mxinden mentioned this pull request Oct 18, 2019

[question] protocols/plaintext: How to use plaintext1 with builder pattern #1277

Closed

peat mentioned this pull request Oct 24, 2019

[mdns] - Support for long mDNS names (Bug #1232) #1287

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement /plaintext/2.0.0 #1236

Implement /plaintext/2.0.0 #1236

ackintosh commented Sep 2, 2019 •

edited

Loading

tomaka left a comment

tomaka Sep 3, 2019

tomaka Sep 3, 2019

tomaka Sep 3, 2019

tomaka Sep 3, 2019

tomaka Sep 3, 2019

ackintosh commented Sep 4, 2019

ackintosh commented Sep 4, 2019

ackintosh commented Sep 6, 2019

ackintosh commented Sep 6, 2019

ackintosh commented Sep 7, 2019 •

edited

Loading

ackintosh commented Sep 7, 2019

tomaka Sep 9, 2019

tomaka Sep 9, 2019

ackintosh Sep 10, 2019

tomaka Sep 9, 2019

ackintosh Sep 10, 2019

tomaka Sep 26, 2019

romanb commented Sep 9, 2019 •

edited

Loading

ackintosh commented Sep 30, 2019

ci/circleci: test-win32

ackintosh commented Sep 30, 2019 •

edited

Loading

ackintosh commented Sep 30, 2019

ackintosh commented Oct 2, 2019

romanb left a comment

romanb Oct 6, 2019

ackintosh commented Oct 11, 2019

romanb commented Oct 11, 2019

tomaka commented Oct 11, 2019

tomaka left a comment


		/// Failed to parse one of the handshake protobuf messages.
		HandshakeParsingFailure,

Implement /plaintext/2.0.0 #1236

Implement /plaintext/2.0.0 #1236

Conversation

ackintosh commented Sep 2, 2019 • edited Loading

tomaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ackintosh commented Sep 4, 2019

ackintosh commented Sep 4, 2019

ackintosh commented Sep 6, 2019

ackintosh commented Sep 6, 2019

ackintosh commented Sep 7, 2019 • edited Loading

ackintosh commented Sep 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romanb commented Sep 9, 2019 • edited Loading

ackintosh commented Sep 30, 2019

ci/circleci: test-win32

ackintosh commented Sep 30, 2019 • edited Loading

ackintosh commented Sep 30, 2019

ackintosh commented Oct 2, 2019

romanb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ackintosh commented Oct 11, 2019

romanb commented Oct 11, 2019

tomaka commented Oct 11, 2019

tomaka left a comment

Choose a reason for hiding this comment

ackintosh commented Sep 2, 2019 •

edited

Loading

ackintosh commented Sep 7, 2019 •

edited

Loading

romanb commented Sep 9, 2019 •

edited

Loading

ackintosh commented Sep 30, 2019 •

edited

Loading